Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix the false 'defined here' messages #82220

Merged
merged 1 commit into from
Feb 25, 2021

Conversation

henryboisdequin
Copy link
Contributor

@henryboisdequin henryboisdequin commented Feb 17, 2021

Closes #80853.

Take this code:

struct S;

fn repro_ref(thing: S) {
    thing();
}

Previously, the error message would be this:

error[E0618]: expected function, found `S`
 --> src/lib.rs:4:5
  |
3 | fn repro_ref(thing: S) {
  |              ----- `S` defined here
4 |     thing();
  |     ^^^^^--
  |     |
  |     call expression requires function

error: aborting due to previous error

This is incorrect as S is not defined in the function arguments, thing is defined there. With this change, the following is emitted:

error[E0618]: expected function, found `S`
  --> $DIR/80853.rs:4:5
   |
LL | fn repro_ref(thing: S) {
   |              ----- is of type `S`
LL |     thing();
   |     ^^^^^--
   |     |
   |     call expression requires function
   |
   = note: local variable `S` is not a function

error: aborting due to previous error

As you can see, this error message points out that thing is of type S and later in a note, that S is not a function. This change does seem like a downside for some error messages. Take this example:

LL | struct Empty2;
   | -------------- is of type `Empty2`

As you can see, the error message shows that the definition of Empty2 is of type Empty2. Although this isn't wrong, it would be more helpful if it would say something like this (which was there previously):

LL | struct Empty2;
   | -------------- `Empty2` defined here

If there is a better way of doing this, where the Empty2 example would stay the same as without this change, please inform me.

Update: This is now fixed

CC @camelid

@rust-highfive
Copy link
Collaborator

r? @varkor

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 17, 2021
Copy link
Contributor

@pickfire pickfire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if adding the note is more helpful since it felt redundant. The error at the top already says so.

expected function, found `Empty2`

And we show

note: unit struct `Empty2` is not a function

If it errors with "expected function" means Empty2 is not a function right? Why do we need to repeat?

@henryboisdequin
Copy link
Contributor Author

I don't know if adding the note is more helpful since it felt redundant. The error at the top already says so.

expected function, found `Empty2`

And we show

note: unit struct `Empty2` is not a function

If it errors with "expected function" means Empty2 is not a function right? Why do we need to repeat?

Thanks for the review @pickfire. I agree, I'll remove that redundant note. By doing so, the local variable mistake is also fixed.

@henryboisdequin
Copy link
Contributor Author

@varkor This PR is now ready for review.

@henryboisdequin henryboisdequin changed the title add helpful error notes and fix the false 'defined here' messages fix the false 'defined here' messages Feb 18, 2021
compiler/rustc_typeck/src/check/callee.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/callee.rs Outdated Show resolved Hide resolved
@henryboisdequin
Copy link
Contributor Author

@rustbot label +S-waiting-on-author -S-waiting-on-review

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 18, 2021
@henryboisdequin
Copy link
Contributor Author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 19, 2021
@varkor
Copy link
Member

varkor commented Feb 19, 2021

If you could address the last two comments and squash the commits, I think this is good to go!

@camelid
Copy link
Member

camelid commented Feb 19, 2021

There is another case that I don't think this handles:

const FOO: usize = 0;

fn main() {
    FOO();
}

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
error[E0618]: expected function, found `usize`
 --> src/main.rs:4:5
  |
1 | const FOO: usize = 0;
  | --------------------- `usize` defined here
...
4 |     FOO();
  |     ^^^--
  |     |
  |     call expression requires function

error: aborting due to previous error

For more information about this error, try `rustc --explain E0618`.
error: could not compile `playground`

To learn more, run the command again with --verbose.

@camelid
Copy link
Member

camelid commented Feb 19, 2021

Perhaps the fix would be also checking for Res::Def where DefKind::ns is ValueNS?

@henryboisdequin
Copy link
Contributor Author

@rustbot label +S-waiting-on-author -S-waiting-on-review

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 20, 2021
@henryboisdequin
Copy link
Contributor Author

Sorry for taking a while, I was on vacation. @varkor, I believed I have addressed all your comments. @camelid I did the check that you have suggested but it does bring us back to the original problem, like this:

LL | struct Empty2;
   | -------------- is of type `Empty2`

It should say Empty2 defined here. Is there a better way to do this? Also, is there a way to get the name of a const to be able to say FOO has type usize instead of -- has type usize. Thanks

@rust-log-analyzer

This comment has been minimized.

@henryboisdequin
Copy link
Contributor Author

The job mingw-check failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)

@pickfire, should I just change it to where it was before

Copy link
Contributor

@pickfire pickfire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@varkor
Copy link
Member

varkor commented Feb 23, 2021

This looks great @henryboisdequin, thanks! Could you squash your commits? Then it's good to go.

@henryboisdequin
Copy link
Contributor Author

@varkor Commits are squashed ✅

@varkor
Copy link
Member

varkor commented Feb 24, 2021

@henryboisdequin: there are some commits that don't appear to be squashed. It's fine to separate different changes into separate commits, but when later commits are reverting changes in previous commits, it just creates churn (which makes tools like git blame less convenient). This change is small enough that it's readable as only one commit.

@henryboisdequin
Copy link
Contributor Author

@varkor Done

@varkor
Copy link
Member

varkor commented Feb 25, 2021

@henryboisdequin: thanks for improving this diagnostic!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 25, 2021

📌 Commit 7d85592713c495e4b746c06e78bd8f90ab6653cd has been approved by varkor

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 25, 2021
compiler/rustc_typeck/src/lib.rs Outdated Show resolved Hide resolved
@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 25, 2021
@henryboisdequin
Copy link
Contributor Author

@varkor, @camelid Done ✅

@varkor
Copy link
Member

varkor commented Feb 25, 2021

@camelid: thanks for pointing that out.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 25, 2021

📌 Commit d7cb66d has been approved by varkor

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 25, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 25, 2021
Rollup of 16 pull requests

Successful merges:

 - rust-lang#75807 (Convert core/num/mod.rs to intra-doc links)
 - rust-lang#80534 (Use #[doc = include_str!()] in std)
 - rust-lang#80553 (Add an impl of Error on `Arc<impl Error>`.)
 - rust-lang#81167 (Make ptr::write const)
 - rust-lang#81575 (rustdoc: Name fields of `ResolutionFailure::WrongNamespace`)
 - rust-lang#81713 (Account for associated consts in the "unstable assoc item name colission" lint)
 - rust-lang#82078 (Make char and u8 methods const)
 - rust-lang#82087 (Fix ICE caused by suggestion with no code substitutions)
 - rust-lang#82090 (Do not consider using a semicolon inside of a different-crate macro)
 - rust-lang#82213 (Slices for vecs)
 - rust-lang#82214 (Remove redundant to_string calls)
 - rust-lang#82220 (fix the false 'defined here' messages)
 - rust-lang#82313 (Update normalize.css to 8.0.1)
 - rust-lang#82321 (AST: Remove some unnecessary boxes)
 - rust-lang#82364 (Improve error msgs when found type is deref of expected)
 - rust-lang#82514 (Update Clippy)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6bf4867 into rust-lang:master Feb 25, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 25, 2021
@henryboisdequin henryboisdequin deleted the fixes-80853 branch February 27, 2021 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"call expression requires function" error points to wrong definition
8 participants